Skip to content

Conversation

@martha-johnston
Copy link
Contributor

https://viam.atlassian.net/browse/APP-10023

This PR updates the joint and pose positions so that they currently reflect the arm state when the other arm interface widget is changed

Screen.Recording.2025-11-03.at.11.50.29.PM.mov

}
}

class _BuildJointControlRow extends StatefulWidget {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved a lot of logic out of this widget and up a level, so now this widget is stateless and this widget doesn't have to be concerned with the notifier

Base automatically changed from app-9878/joint-positions to main November 7, 2025 14:45
@martha-johnston martha-johnston marked this pull request as ready for review November 7, 2025 16:48
@martha-johnston martha-johnston requested a review from a team as a code owner November 7, 2025 16:48
static const double _minPosition = 0.0;
static const double _maxPosition = 180.0;

List<double> _jointValues = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you have two copies of _jointValues, one in the RowState and another in the parent widget. It might be better just to have one list you are manipulating for clarity. Even if you do need the original list, you could only pass the values you need into the row like so: startJointValues: _jointValues[index]

),
],
),
JointPositionsWidget(arm: arm),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a good sign it's an effective widget when you are able to delete a lot of code from a file!

}

void _updateControlValue(int index, double value) {
void _updateControlValue(String index, TextEditingController textController, double value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is index still the right variable name here? Maybe something like axis would be better now.

Comment on lines +217 to +222
onValueChanged: (newValue) => _updateControlValue(
'oY',
_textControllers!.oY,
newValue.clamp(_minOrientation, _maxOrientation),
),
onValueChangedEnd: (newValue) async => _isLive ? _updatePose() : () {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of repeated code in _updateControlValue. Is there anyway to make it more efficient? Maybe could you get the newValue.clamp and _textControllers in the function itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could probably move things around, but I personally like that it just updates the value based on the input rather than searching for the right text controller when we know which one to use at this level. could go either way though, I'll look at reworking it

icon: const Icon(Icons.add),
onPressed: () => onValueChanged(value + 0.1),
onPressed: () async {
onValueChanged(value + (max == 1 ? 0.1 : 1.0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q - What does this mean exactly? If the max is 1 we move the arm by smaller increments? Maybe a comment here might be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants